Bug 2059948 - Do not offer recovery codes to Duo users - #2723
Conversation
There was a problem hiding this comment.
Pull request overview
Restricts BMO recovery-code generation to TOTP accounts and rejects forged requests for unsupported MFA providers.
Changes:
- Adds an explicit recovery-code capability enabled only for TOTP.
- Hides Duo recovery controls and adds server-side enforcement.
- Adds Duo regression coverage and updates documentation.
Show a summary per file
| File | Description |
|---|---|
userprefs.cgi |
Validates recovery requests against the account’s provider. |
Bugzilla/MFA.pm |
Defines and enforces the provider capability. |
Bugzilla/MFA/TOTP.pm |
Enables recovery codes for TOTP. |
template/en/default/account/prefs/mfa.html.tmpl |
Conditionally displays recovery controls. |
template/en/default/global/user-error.html.tmpl |
Adds the unsupported-provider error. |
qa/t/2_test_login_duo.t |
Tests hidden controls and forged requests. |
docs/en/rst/using/two-factor-authentication.rst |
Documents recovery codes as TOTP-only. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 7/7 changed files
- Comments generated: 0
- Review effort level: Balanced
|
9e23f77 as a Duo security fix that also had changes inside that remove recovery codes for Duo. Please take a look and if this PR is a duplicate then please close, if not see if anything was missed and update this PR to fill in the gaps. Thanks! |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
9c8ca38 to
2c2de0d
Compare
|
Thanks! I compared this against 9e23f77 and rebased the PR onto current master. The production changes were duplicates, so I removed them and narrowed this PR to the two gaps left behind: correcting the stale user guide and adding Selenium coverage for the hidden Duo recovery control and a forged recovery request that spoofs TOTP. The focused Duo test passes all 35 tests. |
dklawren
left a comment
dklawren
left a comment
There was a problem hiding this comment.
Automated code review. The docs change and the new Duo-recovery assertions look right — the guard they target is real and reachable (userprefs.cgi:744-756 recomputes $mfa from $user->mfa first, so the forged mfa=TOTP is ignored and duo_user_error is thrown), and mfa-recovery is genuinely gone for Duo in mfa.html.tmpl:90-97. All findings below are about test reliability in qa/t/2_test_login_duo.t.
| $sel->type_ok('mfa-duo-user', $config->{admin_user_login}); | ||
| $sel->type_ok('mfa-password', $config->{admin_user_passwd}); | ||
| $sel->click_ok('update'); | ||
| $sel->driver->find_element('//form[@name="userprefsform"]')->submit; |
There was a problem hiding this comment.
Navigation race (medium). Bugzilla::Test::Selenium::wait_for_page_to_load (Selenium.pm:126) only calls set_timeout('page load', ...) — it never blocks. And under W3C, Selenium::Remote::WebElement::submit is execute_script("return arguments[0].submit()"), which returns as soon as navigation starts. The click_ok('update') this replaces routed to WebDriver Element Click, which waits for navigation to complete.
So line 37's click_ok('//a[contains(text(),"Redirect Back")]') now runs against the pre-submit DOM with only the 600ms implicit wait. On a slow Duo round trip that fails with element-not-found or a stale-element error. (Note you did add sleep(2) after the forged form.submit() on line 78, but not here.)
Coverage loss (low). js/account.js disables #update on load whenever #mfa-action exists, and only the #mfa-select-duo / #mfa-disable handlers re-enable it. Submitting the form directly skips that enable path and skips HTML5 constraint validation on #mfa-password / #mfa-duo-user — the test now passes even if either breaks.
Nit. Bugzilla::Test::Selenium already has submit($locator) (Selenium.pm:436), which does the _fix_locator + find_element(...)->submit() dance. $sel->submit('//form[@name="userprefsform"]') would do here and on line 90.
| 'Duo successfully enabled'); | ||
|
|
||
| ok( | ||
| !$sel->is_element_present('mfa-recovery'), |
There was a problem hiding this comment.
No positive control (low). !is_element_present('mfa-recovery') is satisfied by any page that lacks the button. If the navigation race above leaves the browser on the pre-submit page, an error page, or the Duo redirect page, this passes vacuously and the regression it guards goes unnoticed.
Asserting a control element that must be present on the Duo prefs tab — e.g. ok($sel->is_element_present('mfa-disable'), ...) — alongside it makes the absence meaningful.
(For what it's worth, the locator itself is fine: _fix_locator (Selenium.pm:480) makes this an exact @id/@name match, so mfa-recovery-container / mfa-recovery-frame don't false-positive.)
| document.body.appendChild(form); | ||
| form.submit(); | ||
| }); | ||
| sleep(2); |
There was a problem hiding this comment.
Fixed sleep is the only real wait (low). Line 79's wait_for_page_to_load does not block (see the comment on line 35), and title_is on line 80 is a plain get_title with no retry. If the forged POST takes longer than 2s, the test reads the stale User Preferences title and fails spuriously in CI.
A short poll on the title or body text would be deterministic instead of timing-dependent.
| $sel->click_ok('mfa-disable'); | ||
| $sel->type_ok('mfa-password', $config->{admin_user_passwd}); | ||
| $sel->click_ok('update'); | ||
| $sel->driver->find_element('//form[@name="userprefsform"]')->submit; |
There was a problem hiding this comment.
Same navigation race as line 35: ->submit returns as soon as navigation starts, line 91's wait_for_page_to_load is a no-op, and line 92 immediately clicks //a[contains(text(),"Redirect Back")]. Whatever fix you apply above applies here too.
Commit 9e23f77 landed the production fix that prevents Duo users from generating BMO recovery codes. This follow-up fills the remaining gaps:
Test:
qa/t/2_test_login_duo.t(35 tests)https://bugzilla.mozilla.org/show_bug.cgi?id=2059948